Refactor StorageImpl #123

luckychess · 2019-06-15T15:18:04Z

Description of the Change

StorageImpl refactoring:

PoolWrapper is passed as an external dependency
Postgres initialization code is moved into PgConnectionInit class
Corresponding fixes in Application and tests

Benefits

Connections could be created outside the storage now which is very convenient for testing and (coming) PostgresBlockStorage.

Possible Drawbacks

Pull request is huge :(

- PoolWrapper is passed as an external dependency - Postgres initialization code is moved into PgConnectionInit class - Corresponding fixes in Application and tests Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

MBoldyrev

Some points in my comments may be discussed, so I neither approve, nor demand changes.

MBoldyrev · 2019-06-17T06:12:10Z

irohad/ametsuchi/impl/failover_callback.cpp

+
+FailoverCallback::FailoverCallback(
+    soci::session &connection,
+    std::function<void(soci::session &)> init,


why have you removed the original using InitFunctionType?

MBoldyrev · 2019-06-17T06:19:59Z

irohad/ametsuchi/impl/failover_callback_factory.hpp

+
+namespace iroha {
+  namespace ametsuchi {
+    class FailoverCallbackFactory {


I think the factory name is quite misleading. Factories tend to facilitate objects creation, while this one just passes all the arguments to the object's constructor.
What this class does is lifetime management.
Thus i would suggest renaming this class to something like FailoverCallbacksManager.

Thanks for renaming it into FailoverCallbackHolder, I like your variant better than my ...Manager. There are some more substitutions to be made though:

https://github.com/hyperledger/iroha/blob/d01018a6c1373ee3fca8cf95975a063eda0a3904/irohad/main/impl/pg_connection_init.cpp#L74

MBoldyrev · 2019-06-17T06:30:53Z

irohad/ametsuchi/impl/pool_wrapper.hpp

+
+#include <memory>
+
+#include <soci/soci.h>


can this include be avoided using a forward declaration, like you did for FailoverCallbackFactory?

MBoldyrev · 2019-06-17T06:34:56Z

irohad/ametsuchi/impl/storage_impl.cpp

+      log_manager_(std::move(log_manager)),
+      log_(log_manager_->getLogger()),
+      pool_size_(pool_size),
+      prepared_blocks_enabled_(enable_prepared_blocks),


the enable_prepared_blocks constructor argument seems duplicating a field of pool_wrapper:

Suggested change

prepared_blocks_enabled_(enable_prepared_blocks),

prepared_blocks_enabled_(pool_wrapper_->enable_prepared_transactions_),

MBoldyrev · 2019-06-17T07:11:45Z

irohad/ametsuchi/impl/storage_impl.cpp

+
+void StorageImpl::tryRollback(soci::session &session) {
+  if (block_is_prepared_) {
+    PgConnectionInit::rollbackPrepared(session, prepared_block_name_)


looks weird...
do you have a TODO to separate schema manager from connection initializer?

Nope. Do we already have a task for it?

I am strongly for creating one. Schema management should be separated from connection initialization and generic storage impl.

MBoldyrev · 2019-06-17T08:00:09Z

test/system/irohad_test_data/config.sample.copy

@@ -0,0 +1,11 @@
+{


What did you add this file for?

I didn't. This is pretty annoying, some test modifies this file. I will fix it one day.

Well, it wasn't in .gitignore for some reason so easy fix is coming.

MBoldyrev · 2019-06-17T08:20:52Z

irohad/main/impl/pg_connection_init.hpp

+    }
+
+    inline void processPqNotice(void *arg, const char *message) {
+      auto *log = reinterpret_cast<logger::Logger *>(arg);


Since you use logger::Logger, you should include logger/logger.hpp.
But I would suggest to move this to cpp file and to ensure that it has this include, because otherwise the CMake logger target dependency must be manually added to every file using this header.
If you choose to follow my advise, please include #include "logger/logger_fwd.hpp" to this file.

MBoldyrev · 2019-06-17T08:23:54Z

irohad/main/CMakeLists.txt

+target_link_libraries(pg_connection_init
+    SOCI::postgresql
+    SOCI::core
+    fmt


Suggested change

fmt

logger

The real dependency is logger, and the fact that it compiles without it is a lucky chance.

irohad/main/impl/pg_connection_init.cpp

MBoldyrev · 2019-06-17T08:25:27Z

irohad/main/impl/pg_connection_init.hpp

+#include "ametsuchi/reconnection_strategy.hpp"
+#include "common/result.hpp"
+#include "interfaces/permissions.hpp"
+#include "logger/logger_manager.hpp"


Suggested change

#include "logger/logger_manager.hpp"

#include "logger/logger_manager_fwd.hpp"

- Fixes in naming - Log dependencies - Couple of other small fixes Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

MBoldyrev · 2019-06-18T05:32:34Z

.gitignore

@@ -31,8 +31,8 @@ include/generated/*
 iroha.conf
 peers.list
 cmake-build*
+test/system/irohad_test_data/config.sample.copy


This file is in the repo and hence is versioned with other source. I think for your purposes you should use .git/info/exclude, which is your private and does not synchronize.

Ssssssssssss! I had to check this.
But anyway, it looks like a bad test design.

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron

Looks great overall, thank you! However, some issues have to be addressed before the merge.

lebdron · 2019-06-18T05:00:01Z

irohad/main/application.cpp

@@ -175,13 +177,17 @@ void Irohad::dropStorage() {
  storage->reset();
 }

+// Irohad::RunResult Irohad::initPoolWrapper() {}


Please remove commented-out code.

lebdron · 2019-06-18T05:00:57Z

irohad/main/application.hpp

@@ -152,6 +154,8 @@ class Irohad {
 protected:
  // -----------------------| component initialization |------------------------

+  //  virtual RunResult initPoolWrapper();


Same for commented-out code.

lebdron · 2019-06-18T05:01:28Z

test/module/irohad/ametsuchi/storage_init_test.cpp

@@ -45,6 +47,8 @@ class StorageInitTest : public ::testing::Test {
  std::string pg_opt_without_dbname_;
  std::string pgopt_;

+  // std::shared_ptr<iroha::ametsuchi::PoolWrapper> pool_wrapper_;


Same for commented-out code.

lebdron · 2019-06-18T05:06:43Z

irohad/ametsuchi/impl/failover_callback.hpp

+#include <soci/postgresql/soci-postgresql.h>
+
+#include "ametsuchi/reconnection_strategy.hpp"
+#include "failover_callback.hpp"


Looks like self-include, please remove.

irohad/ametsuchi/impl/failover_callback.hpp

lebdron · 2019-06-18T09:32:46Z

irohad/main/impl/pg_connection_init.hpp

+  namespace ametsuchi {
+    class PgConnectionInit {
+     public:
+      static std::string formatPostgresMessage(const char *message);


Seems to be used only in pg_connection_init.cpp, maybe move it there in an anonymous namespace? Same for processPqNotice

lebdron · 2019-06-18T09:41:22Z

irohad/main/impl/pg_connection_init.cpp

+    ReconnectionStrategyFactory &reconnection_strategy_factory,
+    const PostgresOptions &options,
+    logger::LoggerManagerTreePtr log_manager) {
+  const int pool_size = 10;


I would suggest to make it a parameter as it was before, and define this variable in Irohad::initStorage

lebdron · 2019-06-18T09:42:51Z

irohad/main/impl/pg_connection_init.cpp

+
+  auto conn = initPostgresConnection(options_str, pool_size);
+  if (auto e = boost::get<expected::Error<std::string>>(&conn)) {
+    return expected::makeError(std::move(e->error));


Suggested change

return expected::makeError(std::move(e->error));

return *e;

lebdron · 2019-06-18T09:47:22Z

irohad/main/application.hpp

@@ -25,6 +25,8 @@ namespace iroha {
    class WsvRestorer;
    class TxPresenceCache;
    class Storage;
+    class ReconnectionStrategyFactory;
+    struct PoolWrapper;


Probably can be removed.

ReconnectionStrategyFactory still has to stay.

lebdron · 2019-06-18T09:55:47Z

test/module/irohad/ametsuchi/ametsuchi_fixture.hpp

@@ -50,13 +51,42 @@ namespace iroha {
            std::make_unique<InMemoryBlockStorageFactory>();
        auto reconnection_strategy_factory = std::make_unique<


Should this variable be made fixture static field? Otherwise its lifetime is not managed.

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

lebdron

🎉

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

…ncremental updates to our proc macros Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>

Fix/dependencies

Refactor StorageImpl:

fd1f428

- PoolWrapper is passed as an external dependency - Postgres initialization code is moved into PgConnectionInit class - Corresponding fixes in Application and tests Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

luckychess requested review from MBoldyrev and lebdron June 15, 2019 15:18

MBoldyrev reviewed Jun 17, 2019

View reviewed changes

luckychess added 4 commits June 17, 2019 23:46

Fix review issues:

ff01e4d

- Fixes in naming - Log dependencies - Couple of other small fixes Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

Fix tests

564b59f

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

Move failover_callback into a separate target

bd52852

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

Move pool_wrapper into a separate target

d01018a

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

MBoldyrev reviewed Jun 18, 2019

View reviewed changes

Restore namespaces in StorageImpl

4281d7e

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron suggested changes Jun 18, 2019

View reviewed changes

luckychess added 2 commits June 18, 2019 18:25

Remove wrong .gitignore rule.

1c63faf

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

Fix review issues

c0d0dcf

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>

lebdron approved these changes Jun 19, 2019

View reviewed changes

Fix non-virtual dtor error in FailoverCallback

685e4fe

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

MBoldyrev approved these changes Jun 19, 2019

View reviewed changes

Merge branch 'master' into feature/storage-refactor

6d88130

luckychess merged commit 5430063 into hyperledger-iroha:master Jun 19, 2019

luckychess deleted the feature/storage-refactor branch June 19, 2019 20:36

DCNick3 added a commit to DCNick3/iroha that referenced this pull request Jul 20, 2023

[refactor] hyperledger-iroha#123: Add syn 2.0 to workspace to allow i…

d8d21ed

…ncremental updates to our proc macros Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>

appetrosyan pushed a commit to DCNick3/iroha that referenced this pull request Jul 24, 2023

[refactor] hyperledger-iroha#123: Add syn 2.0 to workspace to allow i…

830ee77

…ncremental updates to our proc macros Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>

nvzhu pushed a commit to soramitsu/soramitsu-iroha that referenced this pull request Jan 9, 2024

Merge pull request hyperledger-iroha#123 from ERussel/fix/dependencies

038e2e6

Fix/dependencies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor StorageImpl #123

Refactor StorageImpl #123

luckychess commented Jun 15, 2019

MBoldyrev left a comment

MBoldyrev Jun 17, 2019

MBoldyrev Jun 17, 2019

MBoldyrev Jun 18, 2019

MBoldyrev Jun 17, 2019

MBoldyrev Jun 17, 2019

MBoldyrev Jun 17, 2019

luckychess Jun 17, 2019

MBoldyrev Jun 17, 2019

MBoldyrev Jun 17, 2019

luckychess Jun 17, 2019

luckychess Jun 17, 2019

MBoldyrev Jun 17, 2019

MBoldyrev Jun 17, 2019 •

edited

Loading

MBoldyrev Jun 17, 2019

MBoldyrev Jun 18, 2019

luckychess Jun 18, 2019

lebdron left a comment

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

lebdron Jun 18, 2019

luckychess Jun 18, 2019

lebdron Jun 18, 2019

lebdron left a comment

	prepared_blocks_enabled_(enable_prepared_blocks),
	prepared_blocks_enabled_(pool_wrapper_->enable_prepared_transactions_),

	#include "logger/logger_manager.hpp"
	#include "logger/logger_manager_fwd.hpp"

		@@ -50,13 +51,42 @@ namespace iroha {
		std::make_unique<InMemoryBlockStorageFactory>();
		auto reconnection_strategy_factory = std::make_unique<

Refactor StorageImpl #123

Refactor StorageImpl #123

Conversation

luckychess commented Jun 15, 2019

Description of the Change

Benefits

Possible Drawbacks

MBoldyrev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBoldyrev Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron left a comment

Choose a reason for hiding this comment

MBoldyrev Jun 17, 2019 •

edited

Loading